Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

fix: don't refresh AuthenticatedAt for pin login (PS-333) #6

Merged
merged 1 commit into from
Jun 17, 2024
Merged

fix: don't refresh AuthenticatedAt for pin login (PS-333) #6

merged 1 commit into from
Jun 17, 2024

Conversation

maoanran
Copy link

@maoanran maoanran commented Jun 3, 2024

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@maoanran maoanran changed the title fix: Don't refresh current session is the session is active (PS-333) fix: don't refresh current session is the session is active (PS-333) Jun 3, 2024
@maoanran maoanran changed the title fix: don't refresh current session is the session is active (PS-333) fix: don't refresh current session if the session is active (PS-333) Jun 3, 2024
splaunov
splaunov requested changes Jun 4, 2024
View reviewed changes
@maoanran maoanran changed the title fix: don't refresh current session if the session is active (PS-333) fix: don't refresh AuthenticatedAt for pin login (PS-333) Jun 7, 2024
splaunov
splaunov approved these changes Jun 10, 2024
View reviewed changes
Copy link

@splaunov splaunov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.
Make sure all tests pass with the command:

go test -p 1 -tags sqlite,json1 -count=1 -failfast -timeout 30m ./...

@maoanran maoanran marked this pull request as ready for review June 10, 2024 11:53
@maoanran maoanran requested a review from aeneasr as a code owner June 10, 2024 11:53
@maoanran
Copy link
Author

Looks good. Make sure all tests pass with the command:

go test -p 1 -tags sqlite,json1 -count=1 -failfast -timeout 30m ./...

Thanks, there are some test failed, but not sure if it is my environment issue, do you know the failures?

--- FAIL: TestSettingsStrategy (1.04s)
    strategy_helper_test.go:246: Environment did not provide Ory Hydra, starting fresh.
    strategy_helper_test.go:270:
        	Error Trace:	/Users/anma/repo/kratos/selfservice/strategy/oidc/strategy_helper_test.go:270
        	            				/Users/anma/repo/kratos/selfservice/strategy/oidc/strategy_settings_test.go:60
        	Error:      	Received unexpected error:
        	            	API error (400): invalid tag format

        	            	github.com/ory/dockertest/v3.(*Pool).RunWithOptions
        	            		/Users/anma/go/pkg/mod/github.com/ory/dockertest/v3@v3.9.1/dockertest.go:413
        	            	github.com/ory/kratos/selfservice/strategy/oidc_test.newHydra
        	            		/Users/anma/repo/kratos/selfservice/strategy/oidc/strategy_helper_test.go:252
        	            	github.com/ory/kratos/selfservice/strategy/oidc_test.TestSettingsStrategy
        	            		/Users/anma/repo/kratos/selfservice/strategy/oidc/strategy_settings_test.go:60
        	            	testing.tRunner
        	            		/opt/homebrew/Cellar/go/1.22.2/libexec/src/testing/testing.go:1689
        	            	runtime.goexit
        	            		/opt/homebrew/Cellar/go/1.22.2/libexec/src/runtime/asm_arm64.s:1222
        	Test:       	TestSettingsStrategy
FAIL
FAIL	github.com/ory/kratos/selfservice/strategy/oidc	19.275s

--- FAIL: TestOAuth2Provider (1.11s)
    op_helpers_test.go:153:
        	Error Trace:	/Users/anma/repo/kratos/selfservice/strategy/password/op_helpers_test.go:153
        	            				/Users/anma/repo/kratos/selfservice/strategy/password/op_login_test.go:206
        	Error:      	Received unexpected error:
        	            	API error (400): invalid tag format

        	            	github.com/ory/dockertest/v3.(*Pool).RunWithOptions
        	            		/Users/anma/go/pkg/mod/github.com/ory/dockertest/v3@v3.9.1/dockertest.go:413
        	            	github.com/ory/kratos/selfservice/strategy/password_test.newHydra
        	            		/Users/anma/repo/kratos/selfservice/strategy/password/op_helpers_test.go:134
        	            	github.com/ory/kratos/selfservice/strategy/password_test.TestOAuth2Provider
        	            		/Users/anma/repo/kratos/selfservice/strategy/password/op_login_test.go:206
        	            	testing.tRunner
        	            		/opt/homebrew/Cellar/go/1.22.2/libexec/src/testing/testing.go:1689
        	            	runtime.goexit
        	            		/opt/homebrew/Cellar/go/1.22.2/libexec/src/runtime/asm_arm64.s:1222
        	Test:       	TestOAuth2Provider
FAIL
FAIL	github.com/ory/kratos/selfservice/strategy/password	4.619s

@splaunov
Copy link

Try to pull hydra with this command before running tests

docker pull oryd/hydra:v2.2.0@sha256:6c0f9195fe04ae16b095417b323881f8c9008837361160502e11587663b37c09

@splaunov splaunov force-pushed the feature/pin-login branch from b88d0fb to 302c013 Compare June 11, 2024 14:57
@gitstream-cm gitstream-cm bot removed the XL review label Jun 17, 2024
@maoanran
Copy link
Author

Thanks, the tests passed.

@maoanran maoanran merged commit 5c81ff5 into feature/pin-login Jun 17, 2024
20 of 22 checks passed
@maoanran maoanran deleted the PS-333 branch June 17, 2024 14:16
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@splaunov splaunov splaunov approved these changes

@aeneasr aeneasr Awaiting requested review from aeneasr aeneasr is a code owner

Assignees
No one assigned
Labels
XS review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maoanran @splaunov